Skip to content

Prevent cloud users from updating their user profile#132622

Merged
azasypkin merged 4 commits intoelastic:feature/user-profilefrom
azasypkin:issue-129238-prevent-cloud-users
Jun 1, 2022
Merged

Prevent cloud users from updating their user profile#132622
azasypkin merged 4 commits intoelastic:feature/user-profilefrom
azasypkin:issue-129238-prevent-cloud-users

Conversation

@azasypkin
Copy link
Copy Markdown
Contributor

@azasypkin azasypkin commented May 20, 2022

Summary

Prevent cloud users from updating their user profile.

This PR is currently based on the not-merged-yet #127624.

Fixes: #129238

@azasypkin azasypkin added blocked Team:Security Platform Security: Auth, Users, Roles, Spaces, Audit Logging, etc t// release_note:skip Skip the PR/issue when compiling release notes v8.3.0 Feature:Security/User Profile ci:no-auto-commit Disable auto-committing changes on CI labels May 20, 2022
@azasypkin azasypkin force-pushed the issue-129238-prevent-cloud-users branch from 9b2b3ea to 5b3fe2a Compare May 23, 2022 08:17
@azasypkin azasypkin force-pushed the issue-129238-prevent-cloud-users branch from 5b3fe2a to d7009c2 Compare May 24, 2022 10:11
@azasypkin azasypkin marked this pull request as ready for review May 24, 2022 11:28
@azasypkin azasypkin requested review from a team as code owners May 24, 2022 11:28
@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented May 24, 2022

Pinging @elastic/kibana-security (Team:Security)

@azasypkin azasypkin mentioned this pull request May 24, 2022
Merged
@azasypkin azasypkin added v8.4.0 and removed v8.3.0 labels May 24, 2022
pgayvallet
pgayvallet approved these changes May 24, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@pgayvallet pgayvallet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

x-pack/plugins/cloud/server/plugin.ts
Comment on lines +53 to +55
if (isCloudEnabled) {
security?.setIsElasticCloudDeployment();
}
Copy link
Copy Markdown
Contributor

@pgayvallet pgayvallet May 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT/optional: Shame on us, we don't yet have a test file for x-pack/plugins/cloud/server/plugin.ts, but it would make sense to add one to test this call/behavior

Copy link
Copy Markdown
Contributor Author

@azasypkin azasypkin May 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good point, I'll add one.

Copy link
Copy Markdown
Contributor Author

@azasypkin azasypkin May 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added few basic tests in c26c503

thomheymann
thomheymann approved these changes May 24, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@thomheymann thomheymann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

I've not been able to test cloud deployment detection logic. Tested locally by mocking elastic_cloud_user response since I've not been able to run Kibana as a cloud user.

@azasypkin
Copy link
Copy Markdown
Contributor Author

azasypkin commented May 25, 2022

I've not been able to test cloud deployment detection logic.

Let me see if we can leverage ci:deploy-cloud label for that, not sure if it works for PRs against feature branches, but it's worth checking.

@afharo
Copy link
Copy Markdown
Member

afharo commented May 25, 2022

Let me see if we can leverage ci:deploy-cloud label for that, not sure if it works for PRs against feature branches, but it's worth checking.

@azasypkin AFAIK, even while it's deployed to Cloud, the user login that we get to test is an ES-authenticated user. We cannot use "Log in with Elastic Cloud" in those deployments. So, essentially, we cannot test it :/

@thomheymann
Copy link
Copy Markdown
Contributor

thomheymann commented May 26, 2022

@afharo Thanks for confirming this is currently not possible. Who would be the best team to raise this issue with? Cloud is an important driver for us and it's not really acceptable to merge features in without being able to test them.

@afharo
Copy link
Copy Markdown
Member

afharo commented May 26, 2022

I assume @elastic/kibana-operations might be able to change the way we deploy clusters from PRs to allow us access?

Also, I'm aware of an effort to allow us to write tests that will use Cloud-authenticated users (also owned by operations) :)

@spalger
Copy link
Copy Markdown
Contributor

spalger commented May 26, 2022

@jbudz, do you know if there are any plans or possible methods available to us to enable testing using cloud SSO auth for cloud instances based on PRs?

@jbudz
Copy link
Copy Markdown
Contributor

jbudz commented May 26, 2022

We don't have any upstream tooling yet to support automating endpoint setup. There's a manual option that we could go through for testing if we want here.

@thomheymann
Copy link
Copy Markdown
Contributor

thomheymann commented May 26, 2022

We don't have any upstream tooling yet to support automating endpoint setup. There's a manual option that we could go through for testing if we want here.

@jbudz Amazing, yeh, even manual would be fine for now. I just want to verify the logic works as expected.

@azasypkin
Copy link
Copy Markdown
Contributor Author

azasypkin commented Jun 1, 2022 •
edited
Loading

@jbudz kindly helped me to test the PR using the Kibana Cloud CI account and I confirmed that we correctly determine ESS managed users and don't allow updating their profiles. The only thing we need to handle is to not render avatar editor for such users since they cannot save changes anyway (added as a point to #132645).

I'll merge PR as soon as CI is happy (ignoring irrelevant failed "Build and Deploy to Cloud" task).

@kibana-ci
Copy link
Copy Markdown

kibana-ci commented Jun 1, 2022 •
edited
Loading

💔 Build Failed

Failed CI Steps

Metrics [docs]

‼️ ERROR: no builds found for mergeBase sha [fb6ab09]

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@azasypkin azasypkin merged commit 705a7d8 into elastic:feature/user-profile Jun 1, 2022
@azasypkin azasypkin deleted the issue-129238-prevent-cloud-users branch June 1, 2022 10:22
@tylersmalley tylersmalley added ci:cloud-deploy Create or update a Cloud deployment and removed ci:deploy-cloud labels Aug 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@thomheymann thomheymann thomheymann approved these changes

@pgayvallet pgayvallet pgayvallet approved these changes

Assignees

No one assigned

Labels

blocked ci:cloud-deploy Create or update a Cloud deployment ci:no-auto-commit Disable auto-committing changes on CI Feature:Security/User Profile release_note:skip Skip the PR/issue when compiling release notes Team:Security Platform Security: Auth, Users, Roles, Spaces, Audit Logging, etc t// v8.4.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@azasypkin @elasticmachine @afharo @thomheymann @spalger @jbudz @kibana-ci @pgayvallet @tylersmalley